Skip to content
This repository has been archived by the owner on May 24, 2022. It is now read-only.

fix: Relates to #138. Prevents user from importing account that is already in account list #327

Merged

Conversation

ltfschoen
Copy link
Contributor

  • Prevents user from importing account and show the message in the screenshot below when they go to "Import account" and select a JSON Backup Keyfile whose address is already associated with an account that is already in the account list

  • Prevents user from importing account and show the message in the screenshot below when they go to "Import account" and enter a Seed Phrase that corresponds to an address that is associated with an account that is already in the account list

screen shot 2018-12-28 at 2 03 42 pm

Copy link
Collaborator

@amaury1093 amaury1093 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

screenshot 2019-01-02 at 11 56 06

When importing an existing account, the address is truncated by the window. Maybe we can use the AddressShort component to render this?

import { inject, observer } from 'mobx-react';

@light({
accountsInfo: () => accountsInfo$().pipe(withoutLoading())
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@@ -67,6 +90,22 @@ class AccountImportOptions extends Component {
}
};

hasExistingAddressForImport = addressForImport => {
const { accountsInfo } = this.props;
const isExistingAddress = Object.keys(accountsInfo)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use account$, no need for Object.keys anymore since it returns a string[]

Copy link
Collaborator

@Tbaut Tbaut left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the pr! Just a nit from my side.
This should not be addressed by this pr but we should rethink the way we present errors and warning message. The errors at least should be more visible.

if (isExistingAddress) {
this.setState({
isLoading: false,
error: `Account already loaded. Address ${addressForImport} is already in the account list`
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would go for something much shorter along the line:
"Account 0x1234..1234 already listed!"
You don't have to see the whole address here.

@ltfschoen
Copy link
Contributor Author

ltfschoen commented Jan 3, 2019

@Tbaut Yes, agreed. I've just been adopting the way errors are current shown, but I personally like using colour-coded animated toasters. Should we create an issue/opportunity for this?
Here are a couple of libraries that include demos:

@ltfschoen
Copy link
Contributor Author

@amaurymartiny @Tbaut I've pushed commits addressing your review comments

const { accounts } = this.props;
const isExistingAddress = accounts
.map(address => address && address.toLowerCase())
.includes(addressForImport);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would put .toLowecase() on this line. So that the input argument can be in any case.

try {
const json = JSON.parse(jsonString);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do not parse here.

3 lines below, the await setJsonString(jsonString); actually parses the JSON, and createAccountStore.address will hold the parsed address. Do the hasExistingAddressForImport then.

@amaury1093
Copy link
Collaborator

@ltfschoen Yes, I agree the errors are quite ugly. In TxForm, I went with react-final-form to put errors as black tooltips over fields, but that might not be ideal for all types of errors. You can create an issue for us to discuss UI options for errors.

Copy link
Collaborator

@Tbaut Tbaut left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Working well 🎉

@Tbaut
Copy link
Collaborator

Tbaut commented Jan 6, 2019

@ltfschoen can you make sure to switch the labels to "please review" and remove "grumbles" once you think you've addressed all the comments?

@ltfschoen
Copy link
Contributor Author

@ltfschoen can you make sure to switch the labels to "please review" and remove "grumbles" once you think you've addressed all the comments?

Sure!

@ltfschoen
Copy link
Contributor Author

I've merged latest from master

@amaury1093
Copy link
Collaborator

@ltfschoen There are 2 grumbles left (from me) 4d ago. I'll merge this PR as soon as they are addressed.

Copy link
Collaborator

@amaury1093 amaury1093 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@amaury1093 amaury1093 merged commit 4c7e0fb into master Jan 7, 2019
@amaury1093 amaury1093 deleted the luke-138-prevent-import-when-account-already-loaded branch January 7, 2019 15:50
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants